refactor(fs): prefer our fs and fs to fs-extra (pt1: non-test files) #5689
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required. |
Contributor
Author
|
/retryBuilds |
kiseCrow
approved these changes
Sep 30, 2024
justinmk3
reviewed
Sep 30, 2024
justinmk3
reviewed
Sep 30, 2024
justinmk3
reviewed
Sep 30, 2024
| config.eventPayloadFile === undefined | ||
| ? undefined | ||
| : JSON.parse(await readFile(config.eventPayloadFile, { encoding: 'utf-8' })) | ||
| : JSON.parse(await fs.readFileAsString(config.eventPayloadFile)) |
Contributor
There was a problem hiding this comment.
off-topic: we should rename this function :)
Suggested change
| : JSON.parse(await fs.readFileAsString(config.eventPayloadFile)) | |
| : JSON.parse(await fs.readFileText(config.eventPayloadFile)) |
Contributor
Author
There was a problem hiding this comment.
Yeah, its not obvious that its auto-converting the file bytes to text. I'll track it, and can do this in a follow up. I think there's enough going on in this PR.
kiseCrow
approved these changes
Sep 30, 2024
justinmk3
reviewed
Sep 30, 2024
| await fs.appendFile(sshConfigPath, section, { mode: 0o600 }) | ||
| const parentsDir = path.dirname(sshConfigPath) | ||
| const grandParentsDir = path.dirname(parentsDir) | ||
| // TODO: replace w/ fs.chmod once stub is merged. |
justinmk3
reviewed
Sep 30, 2024
| fs.copySync(path.join(tempPath, 'usr', 'local', 'sessionmanagerplugin'), outDir, { | ||
| recursive: true, | ||
| }) | ||
| await fs.copy(path.join(tempPath, 'usr', 'local', 'sessionmanagerplugin'), outDir) |
Contributor
There was a problem hiding this comment.
fs.copy needs a docstring, it's not obvious that it also supports folders.
Contributor
Author
There was a problem hiding this comment.
tracked as a followup, seems like there a few other methods that could use them as well in fs.
Co-authored-by: Justin M. Keyes <[email protected]>
Contributor
Author
|
@aws/dexp @aws/codewhisperer-team |
This was referenced Oct 2, 2024
Hweinstock
added a commit
that referenced
this pull request
Oct 4, 2024
## Problem In the recent refactor (#5689), there is a mismatch in args between our fs and fs-extra. Not caught until integ tests were run. ## Solution have default write file method ignores encoding since it uses utf8 anyway. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Hweinstock
added a commit
that referenced
this pull request
Oct 4, 2024
## Problem Follow up from #5689, focusing on the test files. ## Solution Mostly direct translation, one implementation change from `writeJson` to `Json.stringify` then `writeFile` in `packages/core/src/test/awsService/cdk/detectCdkProjects.test.ts` line 120 --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Hweinstock
added a commit
that referenced
this pull request
Oct 7, 2024
…gs (#5713) ## Problem Follow up from: #5689 ## Solution - rename 'readFileAsString' to 'readFileText' since this function assumes a text file (utf-8) encoding. - Add missing doc strings to copy and exist within fs wrapper. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Justin M. Keyes <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We want to avoid fs-extra, and eventually move it to the banned import list. As a first step, we remove fs-extra with as much of our own
fs.tswork, andfswhen not possible.This PR only works on non-testing code. This way we can ensure our tests are not compromised, and still show that our code exhibits the desired behavior.
Solution
Indirect Translations
ensureDirwithmkDiras suggested here: https://github.com/jprichardson/node-fs-extra/blob/HEAD/docs/ensureDir.mdensureDirwith permissions arg can bemkDirwithchmodfollowing.movewithrenamesince we aren't using virtual file system, or crossing disk partitions: https://stackoverflow.com/questions/8579055/how-do-i-move-files-in-node-jsOtherwise the translations are fairly direct.
Notes
As of 9/30 there is a tech debt test failing blocking potential merge. This test was pushed back to 10/30.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.